Skip to content

feat: opt-in list-of-successes grammar engine + dual-engine test infra#490

Merged
SJrX merged 5 commits into
242.xfrom
grammar-parse-engine
Jun 25, 2026
Merged

feat: opt-in list-of-successes grammar engine + dual-engine test infra#490
SJrX merged 5 commits into
242.xfrom
grammar-parse-engine

Conversation

@SJrX

@SJrX SJrX commented Jun 24, 2026

Copy link
Copy Markdown
Owner

What

Adds a second matching method, Combinator.parse(), alongside the existing SyntacticMatch/SemanticMatch on every combinator, plus an opt-in flag to route value validation through it. Nothing in the 200+ production grammars changes, and the original engine stays the default — so there are no user-visible behaviour changes unless the experimental flag is enabled.

Why this design

  • One lenient pass, two answers. The old engine commits to one greedy result over two near-identical passes. parse() returns every way a combinator can proceed (lazily) and folds the strict semantic check into a valid flag per token, so a single pass answers both "could this be colored?" and "is this actually valid?". Greedy traps like Seq(ZeroOrMore("a"), "a") on "aa" just resolve.
  • Failure is a value. A dead end returns a Stuck(offset, expected) rather than an empty result, so error localization (and, later, completion) travels back through the return value with no side channel.

Gating

  • ExperimentalSettings (project service) with a checkbox on the existing "systemd Unit Files" settings page.
  • GrammarOptionValue uses the new engine when the per-project flag is on or FORCE_PARSE_ENGINE (-Dsystemd.unit.grammarParseEngine=true) is set.

Dual-engine test infrastructure

To prove parity, the whole suite runs against both engines:

  • build.gradle.kts forwards the system property to the test task.
  • GitHub Actions gains a grammarParseEngine: [false, true] matrix leg.
  • ci/release.Jenkinsfile re-runs the suite in a parallel pod (separate workspace) with the engine forced on, so every release build validates the experimental path.

Only validation is forced (not cosmetic annotators), so problem counts are identical between engines; only exact error spans can differ. The single such case (SocketBind) is made mode-aware.

Verification

./gradlew test and ./gradlew test -Dsystemd.unit.grammarParseEngine=true both pass locally.

🤖 Generated with Claude Code

Steve Ramage and others added 2 commits June 23, 2026 19:12
…infra

Grows a second matching method, Combinator.parse(), alongside the existing
SyntacticMatch/SemanticMatch on every combinator. Where the old engine commits
to one greedy result over two near-identical passes, parse() returns every way
a combinator can proceed (lazily) and folds the strict semantic check into a
`valid` flag per token, so a single lenient pass answers both "could this be
colored?" and "is this actually valid?". Failure is a value (Stuck) rather than
an empty result, so error localization (and, later, completion) falls out of the
return value with no side channel.

Nothing in the 200+ production grammars changes. The new path is gated behind an
ExperimentalSettings project flag (surfaced as a checkbox on the settings page),
so the original engine remains the default and no user-visible behaviour changes.

To prove parity, GrammarOptionValue.FORCE_PARSE_ENGINE lets the whole unit-test
suite run against the new engine via -Dsystemd.unit.grammarParseEngine=true:
- build.gradle.kts forwards the system property to the test task
- GitHub Actions gains a grammarParseEngine: [false, true] matrix leg
- ci/release.Jenkinsfile runs the suite a second time in a parallel pod with the
  engine forced on, so every release build validates the experimental path

Only validation is forced (not cosmetics), so problem counts are identical and
only exact error spans differ; the one such case (SocketBind) is made mode-aware.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rses

Replace the ":"/"::" comment example (those are two separate terminals in the
grammar, not choices of one) with CollectMode's ("inactive", "inactive-or-failed"),
a real prefix-overlap case, and explain why offering every match avoids greedy
dead-ends. Remove Combinator.fullParses, which was never referenced (validate()
inlines the equivalent full-parse check).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown

Unit Test Results (grammar engine true)

1 135 tests   1 135 ✅  49s ⏱️
  297 suites      0 💤
  297 files        0 ❌

Results for commit e15c358.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown

Unit Test Results (grammar engine false)

1 135 tests   1 135 ✅  46s ⏱️
  297 suites      0 💤
  297 files        0 ❌

Results for commit e15c358.

♻️ This comment has been updated with latest results.

While both engines ship side by side we want full parity coverage of parse() on
every combinator, so GrammarParseTest mirrors GrammarTest method-for-method,
driving the new list-of-successes engine instead of SyntacticMatch/SemanticMatch.
A small View bridge recovers the MatchResult shape so each port reads like the
original (syntactic = longest consumption ignoring validity; semantic = longest
all-valid consumption).

Two intentional differences are asserted and commented rather than hidden:
- FlexibleLiteralChoiceTerminal: the old engine prefers an exact choice over the
  lenient shape regex; parse() currently runs only the regex, so "abcxx" against
  ("abc","defg") matches the greedy "abcx" (invalid) instead of "abc". Worth
  resolving before the engine becomes default.
- Repeat zero-rep matches: the old engine reported longestMatch = 0; the new one
  reports the actual end offset of the empty match.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Steve Ramage and others added 2 commits June 24, 2026 17:07
…iant

When a value is well-formed but invalid, an ambiguous grammar can yield several
full parses that tokenize the same string differently, each with a different
first-invalid token. validate() used to keep whichever the lazy stream yielded
first, so the reported squiggle/quick-fix depended on incidental combinator
iteration order and nothing pinned it.

Report instead the bad token from the parse that stayed valid the longest (max
start offset), mirroring how SyntaxError reports the furthest offset reached.
That rule is invariant under iteration order; the only residual tie — two parses
whose first-invalid token starts at the same offset (two enums over the same
shape) — is broken by stream order, i.e. the earlier-declared alternative, which
an author can steer.

Adds ParseTest coverage: order-invariance across both branch orderings, and the
declaration-order tie-break.

Refs review finding #3 on PR #490.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The tie-break is "first in stream order," which only equals "earlier-declared
alternative" when the tie comes from an AlternativeCombinator. Ties from other
combinators follow their own order (LiteralChoiceTerminal longest-first,
repetitions shorter-count-first), so the previous wording overclaimed that an
author can always steer it by ordering. Clarified both the doc block and the
inline note.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant